src/workers/wallet.ts (1)

Line range hint 177-188: Add error handling when fetching connection info

When retrieving connection information, if getConnection fails, it may cause an unhandled exception. Consider adding error handling to manage potential errors during the asynchronous operations.

Apply this diff to handle possible errors:

 await Promise.all(
   connections.map(async (connection) => {
     try {
       const connectionInfo = await ConnectionController.getConnection(
         this.chain.rest,
         this.chain.chainId,
         connection
       )
       connectionClientMap[connection] = connectionInfo.client_id
+    } catch (error) {
+      this.logger.error(`Failed to get connection info for ${connection}: ${error}`)
+      // Optionally, handle the error or skip this connection
+    }
   })
 )

src/lib/eventParser.spec.ts (1)

Line range hint 1-1: Add test coverage for parseUpdateClientEvent function.

The new parseUpdateClientEvent function lacks test coverage. Consider adding test cases to verify:

• Successful parsing of update_client events
• Handling of missing fields
• Handling of malformed data

Example test structure:

test('update client parser', () => {
  // test successful parsing
  {
    const event = {
      type: 'update_client',
      attributes: [
        { key: 'client_id', value: 'client-1' },
        { key: 'header', value: 'header-data' },
        { key: 'consensus_heights', value: '1-1000' }
      ]
    }
    const result = parseUpdateClientEvent(event)
    expect(result).toEqual({
      clientId: 'client-1',
      header: 'header-data',
      consensusHeights: '1-1000'
    })
  }

  // test missing fields
  {
    const event = {
      type: 'update_client',
      attributes: [
        { key: 'client_id', value: 'client-1' }
      ]
    }
    expect(() => parseUpdateClientEvent(event)).toThrow()
  }
})

src/db/controller/client.spec.ts (2)

8-8: Typo in comment

There's a duplicate word "set" in the comment. Please correct it for clarity.

Apply this diff:

 mockServers // to set set config file
-mockServers // to set set config file
+mockServers // to set config file

---

41-43: Use 'forEach' instead of 'map' for iteration with side effects

Since the result of map is not utilized, using forEach is more appropriate for performing operations with side effects.

Apply this diff:

-testClients.map((client) => {
+testClients.forEach((client) => {
   insert(DB, ClientController.tableName, client)
 })

src/lib/eventParser.ts (1)

132-142: LGTM: Well-structured event parsing implementation.

The parseUpdateClientEvent function follows the established pattern of other event parsers in the file. It correctly extracts the required fields using the existing find helper function.

However, consider adding error handling for missing required fields.

 export function parseUpdateClientEvent(event: Event): UpdateClientEvent {
   const clientId = find(event, 'client_id') as string
   const header = find(event, 'header') as string
   const consensusHeights = find(event, 'consensus_heights') as string
+  
+  if (!clientId || !header || !consensusHeights) {
+    throw new Error('Missing required fields in update client event')
+  }

   return {
     clientId,
     header,
     consensusHeights,
   }
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

 const consensusHeightsArray = event.consensusHeights.split(',')
 const revisionHeightParts = consensusHeightsArray[0].split('-')
+if (revisionHeightParts.length < 2) {
+  throw new Error('Invalid consensusHeights format')
+}
 const revisionHeight = parseInt(revisionHeightParts[1])

src/msgs/updateClient.ts (1)

Line range hint 79-89: Improve error handling in the header retrieval loop.

The current implementation has a few issues:

1. The error is only thrown after 5 retries, but the error message doesn't indicate the retry attempts.
2. The delay between retries is fixed at 100ms without exponential backoff.

Consider this improved implementation:

-  let header = await chain.rpc.header(height).catch(() => undefined)
-  let count = 0
-  while (header === undefined) {
-    header = await chain.rpc.header(height).catch((e) => {
-      if (count > 5) {
-        throw e
-      }
-      return undefined
-    })
-    await delay(100)
-    count++
-  }
+  const MAX_RETRIES = 5;
+  const BASE_DELAY = 100;
+  let retries = 0;
+  
+  while (retries <= MAX_RETRIES) {
+    try {
+      const header = await chain.rpc.header(height);
+      if (header) return header;
+    } catch (e) {
+      if (retries === MAX_RETRIES) {
+        throw new Error(`Failed to fetch header after ${MAX_RETRIES} retries: ${e.message}`);
+      }
+      await delay(BASE_DELAY * Math.pow(2, retries));
+      retries++;
+    }
+  }

src/db/controller/packet.ts (1)

Line range hint 506-513: Critical: Incorrect table names used in delete operations.

Several delete operations are using the wrong table names, which will result in deleting records from the wrong tables:

1. In feedAcknowledgePacketEvent:
   • Using tableNamePacketSend instead of tableNamePacketTimeout
   • Using tableNamePacketSend instead of tableNamePacketWriteAck
2. In feedTimeoutPacketEvent:
   • Using tableNamePacketSend instead of tableNamePacketTimeout

Apply these fixes:

-      del<PacketTimeoutTable>(DB, PacketController.tableNamePacketSend, [
+      del<PacketTimeoutTable>(DB, PacketController.tableNamePacketTimeout, [

-      del<PacketWriteAckTable>(DB, PacketController.tableNamePacketSend, [
+      del<PacketWriteAckTable>(DB, PacketController.tableNamePacketWriteAck, [

-      del<PacketTimeoutTable>(DB, PacketController.tableNamePacketSend, [
+      del<PacketTimeoutTable>(DB, PacketController.tableNamePacketTimeout, [

Also applies to: 516-523, 570-577

src/db/controller/client.ts (2)

2-2: Organize Imports for Readability

Consider ordering your imports to enhance readability. Typically, external libraries are imported first, followed by internal modules.

---

69-82: Avoid Magic Strings for Table Names

Using hard-coded table names can lead to maintenance issues. Consider defining table names as constants or using configuration files.

src/db/controller/packet.ts (2)

318-325: Consider using a transaction for atomic updates.

The resetPacketInProgress method updates multiple tables independently. If one update fails, it could leave the system in an inconsistent state.

Consider wrapping the updates in a transaction:

  public static resetPacketInProgress(db?: Database) {
    db = db ?? DB
+   db.transaction(() => {
      update<PacketSendTable>(db, PacketController.tableNamePacketSend, {
        in_progress: Bool.FALSE,
      })
      update<PacketTimeoutTable>(db, PacketController.tableNamePacketTimeout, {
        in_progress: Bool.FALSE,
      })
      update<PacketWriteAckTable>(db, PacketController.tableNamePacketWriteAck, {
        in_progress: Bool.FALSE,
      })
+   })()
  }

---

385-386: Consider using a transaction for atomic inserts.

The feedSendPacketEvent method inserts records into multiple tables independently. If one insert fails, it could leave the system in an inconsistent state.

Consider wrapping the inserts in a transaction:

    return () => {
+     DB.transaction(() => {
        insert(DB, PacketController.tableNamePacketSend, packetSend)
        insert(DB, PacketController.tableNamePacketTimeout, packetTimeout)
+     })()

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

- const client = selectOne<ClientTable>(DB, this.tableName, [
+ const client = selectOne<ClientTable>(DB, ClientController.tableName, [

- const connection = selectOne<ConnectionTable>(
-   DB,
-   this.tableName,
+ const connection = selectOne<ConnectionTable>(
+   DB,
+   ConnectionController.tableName,
    [
      {
        chain_id: chainId,
        connection_id: connectionId,
      },
    ]
  )

src/db/controller/packet.spec.ts (1)

7-7: Typo in the describe block
Please correct "controler" to "controller" for consistent naming throughout the codebase.

Proposed fix:

-describe('channel controler', () => {
+describe('channel controller', () => {

src/lib/rpcClient.ts (1)

135-167: Consider using a for-loop instead of map for side effects.

Within rpcBlockResult.result.finalize_block_events.map((event) => { ... }), the map function is used for iteration rather than generating a transformed array. Switching to a for-loop improves clarity.

- rpcBlockResult.result.finalize_block_events.map((event) => {
+ for (const event of rpcBlockResult.result.finalize_block_events) {

src/db/controller/packetFee.spec.ts (1)

8-10: Typo in the test description.

Change "packet controler" to "packet controller" for consistency.

-describe('packet controler', () => {
+describe('packet controller', () => {

src/workers/wallet.ts (3)

Line range hint 177-189: Add error handling for connection retrieval.

The connection client mapping logic should handle potential failures when retrieving connection information.

 await Promise.all(
   connections.map(async (connection) => {
+    try {
       const connectionInfo = await ConnectionController.getConnection(
         this.chain.rest,
         this.chain.chainId,
         connection
       )
       connectionClientMap[connection] = connectionInfo.client_id
+    } catch (error) {
+      this.logger.error(`Failed to get connection info for ${connection}: ${error}`)
+      // Skip this connection but allow others to proceed
+    }
   })
 )

---

243-304: Clarify filtering comments and logic.

The comments suggest filtering expired clients, but the logic is actually checking for undefined client IDs in the connection map. This could be misleading for future maintenance.

 filteredSendPackets
   .filter(
     (packet) =>
-      connectionClientMap[packet.dst_connection_id] !== undefined
-    ) // filter expired client
+      connectionClientMap[packet.dst_connection_id] !== undefined
+    ) // filter packets with invalid connection-client mapping

Similar changes should be applied to the filtering logic in ackMsgs, timeoutMsgs, and channelOpenMsgs.

---

718-724: Consider moving utility function to a separate file.

The asyncFilter utility function is well-implemented but should be moved to a separate utilities file for better code organization and reusability.

Consider moving it to src/lib/utils.ts:

// src/lib/utils.ts
export async function asyncFilter<T>(
  array: T[],
  filter: (v: T) => Promise<boolean>
): Promise<T[]> {
  const filterRes = await Promise.all(array.map((v) => filter(v)))
  return array.filter((v, i) => filterRes[i])
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:54-66
Timestamp: 2024-12-17T07:59:34.765Z
Learning: In `src/db/controller/client.ts`, `consensusHeights` must exist and follow the `number-number` format as per the IBC-Go specification. If it doesn't exist or the format is incorrect, the process should throw an error and terminate.

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:100-133
Timestamp: 2024-12-17T07:56:36.084Z
Learning: In the `getClientsToUpdate` method of `ClientController` in `src/db/controller/client.ts`, precise timing calculations are not necessary, and approximate times are acceptable for the client update conditions.

src/workers/wallet.ts (1)

204-217: ⚠️ Potential issue

Fix typo in function name and improve error handling.

The function name has a typo and lacks error handling for client retrieval.

-const fileterExpiredClient = async (clientId: string) => {
+const filterExpiredClient = async (clientId: string) => {
   const currentTimestamp = new Date().valueOf() / 1000

+  try {
     const client = await ClientController.getClient(
       this.chain.rest,
       this.chain.chainId,
       clientId
     )

     return (
-      client.last_update_time + client.trusting_period < currentTimestamp
+      Math.floor(client.last_update_time + client.trusting_period) < Math.floor(currentTimestamp)
     )
+  } catch (error) {
+    this.logger.error(`Failed to get client info for ${clientId}: ${error}`)
+    return true // Consider client as expired if we can't verify its state
+  }
 }

src/workers/wallet.ts (4)

190-194: Add error handling for client update check.

The call to getClientsToUpdate should include error handling to gracefully handle potential failures.

-      const clientsToUpdate = ClientController.getClientsToUpdate(
+      const clientsToUpdate = await ClientController.getClientsToUpdate(
         this.chain.chainId,
         counterpartyChainIdsWithFeeFilter.map((f) => f.chainId)
-      )
+      ).catch(error => {
+        this.logger.error(`Failed to get clients to update: ${error}`);
+        return [];
+      });

---

243-301: Improve code organization and type safety in message generation.

The message filtering and generation code could benefit from the following improvements:

1. Extract common filtering logic to reduce code duplication
2. Add type safety for undefined checks

+interface PacketFilter {
+  connectionId: string;
+  clientId: string;
+  height: Height;
+}
+
+function filterExpiredClientPackets<T extends { connection_id: string }>(
+  packets: T[],
+  connectionClientMap: Record<string, string>,
+  updateClientMsgs: Record<string, { msg: MsgUpdateClient; height: Height }>
+): PacketFilter[] {
+  return packets
+    .filter(packet => {
+      const clientId = connectionClientMap[packet.connection_id];
+      return updateClientMsgs[clientId] !== undefined;
+    })
+    .map(packet => ({
+      connectionId: packet.connection_id,
+      clientId: connectionClientMap[packet.connection_id],
+      height: updateClientMsgs[connectionClientMap[packet.connection_id]].height
+    }));
+}

// Example usage for recv packets:
const recvPacketMsgs = await Promise.all(
  filterExpiredClientPackets(
    filteredSendPackets,
    connectionClientMap,
    updateClientMsgs
  ).map(({ height }) =>
    this.workerController.generateRecvPacketMsg(
      packet,
      height,
      this.address()
    )
  )
);

---

724-730: Add documentation and error handling to asyncFilter.

The helper function could benefit from JSDoc documentation and error handling for individual filter operations.

+/**
+ * Filters an array asynchronously using the provided filter function.
+ * @template T The type of elements in the array
+ * @param {T[]} array The array to filter
+ * @param {(v: T) => Promise<boolean>} filter The async filter function
+ * @returns {Promise<T[]>} A promise that resolves to the filtered array
+ */
 async function asyncFilter<T>(
   array: T[],
   filter: (v: T) => Promise<boolean>
 ): Promise<T[]> {
-  const filterRes = await Promise.all(array.map((v) => filter(v)))
+  const filterRes = await Promise.all(
+    array.map((v) =>
+      filter(v).catch((error) => {
+        console.error(`Filter operation failed: ${error}`);
+        return false;
+      })
+    )
+  );
   return array.filter((v, i) => filterRes[i])
 }

---

352-352: Consider implementing a rate-limiting strategy.

The worker processes messages in batches but doesn't implement a clear backoff strategy. Consider adding rate limiting to prevent overwhelming the chain's RPC endpoints during high load.

Some suggestions:

1. Implement exponential backoff when errors occur
2. Add configurable delays between batches based on chain load
3. Consider using a token bucket algorithm for rate limiting

Would you like me to provide a detailed implementation for any of these approaches?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/lib/restClient.ts (1)

58-62: Consider adding robust error handling for consensus state retrieval.

Currently, if the endpoint is unavailable or returns an unexpected format, the method might throw an unhandled exception. You may want to add a try/catch block or a validation step to ensure the response meets the expected schema before returning it.

src/db/controller/client.ts (1)

142-166: Consider handling invalid or missing data in fetchClient.

If rest.ibc.getClientState(clientId) or rest.ibc.lastConsensusState(clientId) returns invalid data or fails unexpectedly, you risk inserting partially parsed data into the table. Consider validating or wrapping these calls in try/catch to ensure data integrity.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:54-66
Timestamp: 2024-12-17T07:59:34.765Z
Learning: In `src/db/controller/client.ts`, `consensusHeights` must exist and follow the `number-number` format as per the IBC-Go specification. If it doesn't exist or the format is incorrect, the process should throw an error and terminate.

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:100-133
Timestamp: 2024-12-17T07:56:36.084Z
Learning: In the `getClientsToUpdate` method of `ClientController` in `src/db/controller/client.ts`, precise timing calculations are not necessary, and approximate times are acceptable for the client update conditions.

src/workers/wallet.ts (1)

190-203: Ensure parallel retrievals won't exceed resource constraints.
getClientsToUpdate and subsequent array operations might open too many parallel DB calls in large-scale scenarios. Consider adding concurrency limits or batching if performance becomes an issue.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/db/controller/client.ts (2)

68-72: Validate revisionHeight parsing.
If parseInt fails, it returns NaN. Consider adding additional checks for isNaN(revisionHeight) to avoid silently overwriting client.revision_height with NaN.

---

81-94: Efficient database updates.
Your update<ClientTable> approach is clear. Consider adding error handling or a rollback strategy if the update fails to ensure consistency.

src/workers/wallet.ts (3)

204-218: Expiration check logic.
Your check for client.last_update_time + client.trusting_period > currentTimestamp is reversed from the typical "Is it expired?" approach. Here, true means "not expired" so the naming or logic might be confusing.

---

231-231: Error handling for generateMsgUpdateClient.
If generateMsgUpdateClient fails for any reason, you might end up with an unhandled situation. Consider wrapping each call in a try-catch to handle potential REST or parsing errors individually.

---

243-249: Filtering for expired clients.
.filter(...) // filter expired client might omit the reason behind the filter in code-level comments. Consider clarifying so future devs understand the condition precisely.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:54-66
Timestamp: 2024-12-17T07:59:34.765Z
Learning: In `src/db/controller/client.ts`, `consensusHeights` must exist and follow the `number-number` format as per the IBC-Go specification. If it doesn't exist or the format is incorrect, the process should throw an error and terminate.

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:100-133
Timestamp: 2024-12-17T07:56:36.084Z
Learning: In the `getClientsToUpdate` method of `ClientController` in `src/db/controller/client.ts`, precise timing calculations are not necessary, and approximate times are acceptable for the client update conditions.

src/db/controller/client.ts (4)

17-22: Check for database insertion errors.
While this insertion is straightforward, consider wrapping insert in a try-catch block or verifying that the database call succeeds to handle potential DB errors more gracefully.

---

24-37: Ensure robust error handling in replaceClient.
When deleting and re-inserting the client, if either DB call fails, it could corrupt or partially remove the record. Consider transactional operations or a rollback strategy to maintain DB consistency.

---

81-94: Consider logging or returning update success status.
Right now, the function calls update silently. Providing some form of acknowledgment or logging would improve observability to confirm a successful DB update.

---

103-103: Handle potential fetchClient errors.
This usage is fine, but keep in mind that this.addClient(...) could fail if it can’t fetch or insert. A try-catch approach here could stop repetitive DB attempts in infinite loops if rest calls fail.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:54-66
Timestamp: 2024-12-17T07:59:34.765Z
Learning: In `src/db/controller/client.ts`, `consensusHeights` must exist and follow the `number-number` format as per the IBC-Go specification. If it doesn't exist or the format is incorrect, the process should throw an error and terminate.

Learnt from: ALPAC-4
PR: initia-labs/rapid-relayer#20
File: src/db/controller/client.ts:100-133
Timestamp: 2024-12-17T07:56:36.084Z
Learning: In the `getClientsToUpdate` method of `ClientController` in `src/db/controller/client.ts`, precise timing calculations are not necessary, and approximate times are acceptable for the client update conditions.